Measure pool_peak_bytes per iteration and report the smallest - #24023
Open
adriangb wants to merge 1 commit into
Open
Measure pool_peak_bytes per iteration and report the smallest#24023adriangb wants to merge 1 commit into
adriangb wants to merge 1 commit into
Conversation
`pool_peak_bytes` was read in `write_iter`, from a recorder last reset at `start_new_case`. Benchmarks run every iteration of a query before reporting any of them, so by then the recorder held the high-water mark of the whole set: the field was a max over iterations, and each iteration's entry reported the same contaminated number. A peak reservation is a max over the operators holding memory at one moment, so it moves with how the runtime happened to interleave partitions on a given pass. Reducing that with `max` lets one unlucky pass set the number for the query, which makes the field jump between runs of identical code — an A/B comparison over a same-commit branch saw Q11 move +61.0% and Q1 -25.0% while 14 of 22 queries agreed to within 1%, the outliers landing on round multiples of a partition's worth of memory. Take the reading at the end of each iteration instead, via `take_iteration_peak`, and keep the smallest across a query's iterations. The results JSON is unchanged: `pool_peak_bytes` remains one field per query, computed differently. `min` is a judgement call — it answers "how much does this query need when nothing else is in the way", which is what a commit-to-commit comparison wants. Median is a one-line change if that reads better. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #24023 +/- ##
==========================================
- Coverage 80.86% 80.83% -0.03%
==========================================
Files 1099 1099
Lines 374327 374480 +153
Branches 374327 374480 +153
==========================================
+ Hits 302685 302729 +44
- Misses 53596 53707 +111
+ Partials 18046 18044 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Follow-up to #23985, which added
pool_peak_bytes. No open issue.Rationale for this change
pool_peak_bytesis documented as the peak reservation for a query, but it is currently a max across all of that query's iterations, and every iteration's entry carries the same contaminated value.The recorder is reset in
start_new_caseand read inwrite_iter. Those look adjacent, but benchmarks run every iteration of a query before reporting any of them —benchmark_queryreturns aVec<QueryResult>that is then looped over. So by the time the firstwrite_iterruns, the recorder already holds the high-water mark of the whole set. The existing comment states the consequence:A peak reservation is a max over the operators holding memory at the same instant, so it moves with how the runtime happened to interleave partitions on that pass. Reducing across iterations with
maxmeans one unlucky pass sets the number for the query, and the field jumps between runs of identical code.Measured on a same-commit A/B (both sides the same SHA, so every difference is noise), TPC-H SF1, 5 iterations:
+0.0%The distribution is bimodal rather than continuous, and the outliers land on round multiples — Q1 and Q16 both move by exactly 10 MiB. That is a discrete quantity (partitions holding a reservation concurrently) changing by whole units, amplified by taking the largest of 5 draws. It also biases the reported value upward: max-of-5 overstates what a single run reserves.
What changes are included in this PR?
take_iteration_peak(&Arc<dyn MemoryPool>)reads the peak and rebases the recorder, so each iteration is measured on its own.QueryResultcarriespool_peak_bytes; each benchmark's iteration loop populates it where the iteration actually ends.write_itertakes that reading and keeps the smallest across a query's iterations.minis the judgement call here. It answers "how much does this query need when nothing else is in the way", which is what a commit-to-commit comparison wants, and it matches howcompare.pyalready reduces timings when not passed--detailed. Median is a one-line change if maintainers prefer it — happy to switch.Measured effect
Two A/A runs on the benchmark harness at TPC-H SF1, 5 iterations,
DATAFUSION_RUNTIME_MEMORY_LIMIT=4G. Each pins both sides to one commit, so every reported difference is noise: before ona589b4bb, after on this branch.This reduces the noise; it does not remove it. Four of the worst queries collapse — Q13 39.9% -> 0.0%, Q22 23.3% -> 2.8%, Q11 22.7% -> 3.3%, Q21 18.5% -> 6.2% — but Q1 got worse (27.5% -> 50.0%) and the worst case across the set rose.
Q1 is instructive: 20.0 MiB against 30.0 MiB, a difference of exactly one 10 MiB unit. The underlying quantity is discrete and genuinely bimodal — the query reserves either two units or three depending on how partitions interleave — so with only 5 draws,
minstill differs by a whole unit whenever one side never draws the low mode. No reduction over 5 samples fixes that; it needs either more iterations or a deterministic partition count.What the change does deliver unambiguously is that each iteration's reading is its own, and the de-inflation that follows: summed across the 22 queries the reported peak drops from 1381 MiB to 1174 MiB (-15%). That part is mechanical, not sampling — max-of-5 was overstating what a single run reserves.
Caveat: one run per configuration, so the per-query deltas above carry their own noise. The aggregate figures and the counts are the trustworthy part; individual movements like Q1's are not resolvable from n=1.
Are these changes tested?
Yes — unit tests in
util/run.rs, including that the quietest iteration sets the value, that an iteration no longer inherits the previous one's mark, and that bytes held across a reading stay in it (reset_peakrebases on what is reserved, not on zero, so data loaded up front still counts for every iteration that runs with it).Are there any user-facing changes?
The results JSON is unchanged —
pool_peak_bytesremains one field per query, computed differently.compare.pyneeds no change.Reported values are generally lower than before (-15% in aggregate, see above) and noticeably more stable, though not fully reproducible. Anything tracking these numbers over time will see a step at this commit.